Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - Chat Agent Pinning #14716

Merged
merged 20 commits into from
Feb 17, 2025

Conversation

atahankilc
Copy link
Contributor

What it does

An "agent pinning" feature has been introduced to enhance the user experience in consecutive agent usage within the chat interface.

  • Automatic Pinning:
    If a chat agent is mentioned in a prompt and there is no prior pinned agent, the mentioned agent is automatically pinned. This eliminates the need for the user to repeatedly mention the agent in subsequent prompts.

  • Handling New Mentions:
    If a different agent is mentioned while another agent is pinned, the pinned agent remains unchanged. The newly mentioned agent is used only for the specific prompt where it is mentioned, without affecting the pinned agent.

  • Manual Control:
    Users can manually unpin the agent through the chat interface if desired.

  • New Session with Pinned Agent:
    A new chat session can now be initiated directly with a pinned agent.

How to test

  • Mention an agent in chat interface
  • Execute AI_CHAT_NEW_CHAT_WINDOW_WITH_PINNED_AGENT_COMMAND with passing desired chat agent

Follow-ups

As part of this contribution, We encountered an issue with the added command ai-chat-ui.new-chat-with-pinned-agent. The command is expected to take a single argument of type ChatAgent | undefined. However, the function call fails when the argument is passed directly in the expected form. To make the command work, We had to pass an argument array instead, where the first item in the array is undefined, followed by the actual ChatAgent value.

This behavior seems to deviate from the intended design. While this workaround resolves the issue for now, the underlying cause still needs to be investigated. The error message encountered when not using this approach is:

Error: The command 'ai-chat-ui.new-chat-with-pinned-agent' cannot be executed. There are no active handlers available for the command.

We are including this as a follow-up issue to address later and investigate why the command handler does not behave as expected when a single argument is passed directly.

Attribution

This feature was developed by @atahankilc, @ZidongWang123, @TANG839, and @mihaela21k as part of the AI Coding Exercises project in the Practical Course Software Engineering for Business Information Systems at Technical University of Munich.

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming requested a review from planger January 13, 2025 08:35
@JonasHelming
Copy link
Contributor

@atahankilc Could you fix the linting:
@theia/ai-chat-ui: /home/runner/work/theia/theia/packages/ai-chat-ui/src/browser/chat-input-widget.tsx
@theia/ai-chat-ui: 284:1 error Trailing spaces not allowed no-trailing-spaces

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you for your contribution! I only have one main concern, the rest of my comments inline are rather minor.

packages/ai-chat/src/common/chat-service.ts Outdated Show resolved Hide resolved
@planger
Copy link
Contributor

planger commented Jan 29, 2025

As the UI of the chat input has changed quite a bit since this PR started, I'd suggest to integrate the pinned agent as sketched in the following screenshot:

image

We have room below the text input for additional controls and information. For instance, we use a + icon to add context in another PR. I think it'd be great to use this room to put the agent pinning UI there as well, in the form of a @ button to e.g. pin or unpin an agent, and showing the currently pinned agent, if any, next to it.

Buttons shown at this place are currently controlled here in the code:
https://github.com/eclipse-theia/theia/blob/master/packages/ai-chat-ui/src/browser/chat-input-widget.tsx#L322

We may need to extend this a bit to allow also showing an optional text left to the button.

This avoids also any interference with the changesets that may be shown above the chat input:

image

What do you think?

@atahankilc
Copy link
Contributor Author

Hey @planger. Thank you for your guidance. I tried to refactor the pinning UI and related items. I've attached the updated UI to this comment. There's one thing I couldn't manage to sort out and wanted to get your opinion on. Our implementation does not include agent pinning from the input widget component, so I tried to implement it in a way that the "@" icon is displayed by default, which would trigger agent completion pop-up when clicked. However, I got a bit tangled up in the process. As a result, the option is currently only shown if there is a pinned agent, and clicking it only allows unpinning. With this said, is there anything I could do to trigger agent suggestions? And if not, should I implement a separate UI for selecting an agent?

Screenshot 2025-02-03 at 03 10 15

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this iteration! I think this already feels pretty nice. I'm not sure we really need an explicit pin action though. I think we have two options for pinning agents:

  1. Show a context menu on click and offer "Unpin" if pinned, and "Pin > ".
  2. Trigger the auto completion for agents as you mentioned.

I personally prefer option 2. To trigger the code completion, I think a handler like this should work (if pinnedAgent === undefined):

handler: () => {
                    if (editorRef.current) {
                        editorRef.current.getControl().getModel()?.applyEdits([{
                            range: {
                                startLineNumber: 1,
                                startColumn: 1,
                                endLineNumber: 1,
                                endColumn: 1
                            },
                            text: '@ ',
                        }]);
                        editorRef.current.getControl().setPosition({ lineNumber: 1, column: 2 });
                        editorRef.current.getControl().getAction('editor.action.triggerSuggest')?.run();
                    }
                }

packages/ai-chat-ui/src/browser/style/index.css Outdated Show resolved Hide resolved
Comment on lines 82 to 87
registry.registerCommand(AI_CHAT_NEW_CHAT_WINDOW_WITH_PINNED_AGENT_COMMAND, {
// TODO - not working if function arg is set to type ChatAgent | undefined ?
execute: (...args: unknown[]) => this.chatService.createSession(ChatAgentLocation.Panel, {focus: true}, args[1] as ChatAgent | undefined),
isEnabled: widget => this.withWidget(widget, () => true),
isVisible: widget => this.withWidget(widget, () => true),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this command is never invoked anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the extension we are developing for our course, we are using this command to directly open a chat widget with an agent pinned. If you don't have such a functionality expectation, I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think we need this in Theia at the moment.

Comment on lines 41 to 46

export const AI_CHAT_NEW_CHAT_WINDOW_WITH_PINNED_AGENT_COMMAND: Command = {
id: 'ai-chat-ui.new-chat-with-pinned-agent',
iconClass: codicon('add')
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really used at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'd remove this.

packages/ai-chat-ui/src/browser/chat-view-widget.tsx Outdated Show resolved Hide resolved
packages/ai-chat/src/common/chat-service.ts Outdated Show resolved Hide resolved
@planger
Copy link
Contributor

planger commented Feb 3, 2025

Oh, and one more comment. I think we should ensure to show the at-icon only if more than one chat agent is registered -- and make it configurable similar to the showContext:
https://github.com/eclipse-theia/theia/blob/master/packages/ai-chat-ui/src/browser/ai-chat-ui-frontend-module.ts#L64

Because we do have adopters who only provide one chat agent and don't provide the option for users to switch between chat agents.

@atahankilc
Copy link
Contributor Author

Oh, and one more comment. I think we should ensure to show the at-icon only if more than one chat agent is registered -- and make it configurable similar to the showContext: https://github.com/eclipse-theia/theia/blob/master/packages/ai-chat-ui/src/browser/ai-chat-ui-frontend-module.ts#L64

Because we do have adopters who only provide one chat agent and don't provide the option for users to switch between chat agents.

I have added a setting option under ai features to dis/enable this pinning functionality and also made it configurable similar to the showContext, however, I couldn't directly link the preference in the settings to the frontend module. If you find it appropriate, I can write a class/function similar to AIActivationService to enable or disable pinning button in chat widget. This way, I believe I can retrieve the number of currently registered agents from the ChatAgentService or reflect the setting value by getting it from PreferenceService.

@JonasHelming JonasHelming requested a review from planger February 7, 2025 14:47
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the revision! I think we are almost there. Can you please remove the comment and rebase it, there are a few conflicts. But then this is imho ready.

Thank you very much for this nice contribution!

Comment on lines 82 to 87
registry.registerCommand(AI_CHAT_NEW_CHAT_WINDOW_WITH_PINNED_AGENT_COMMAND, {
// TODO - not working if function arg is set to type ChatAgent | undefined ?
execute: (...args: unknown[]) => this.chatService.createSession(ChatAgentLocation.Panel, {focus: true}, args[1] as ChatAgent | undefined),
isEnabled: widget => this.withWidget(widget, () => true),
isVisible: widget => this.withWidget(widget, () => true),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think we need this in Theia at the moment.

Comment on lines 41 to 46

export const AI_CHAT_NEW_CHAT_WINDOW_WITH_PINNED_AGENT_COMMAND: Command = {
id: 'ai-chat-ui.new-chat-with-pinned-agent',
iconClass: codicon('add')
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'd remove this.

@atahankilc
Copy link
Contributor Author

Thanks a lot for your feedbacks. I have removed the code parts you mentioned in your comments and rebased branch. I noticed that DefaultChatAgentId binding was removed from ai-chat-frontend-module.ts, and if PinChatAgent also needs to be removed, I can remove it.. It should be all set for another review. Let me know if anything else needs tweaking.

Appreciate it!

@atahankilc atahankilc requested a review from planger February 14, 2025 12:31
@atahankilc
Copy link
Contributor Author

I have noticed a bug in chat input widget and wanted to share with you:

The height of the chat input box kept increasing continuously, and it seemed to be fixed when I set the height in the theia-ChatInput-Editor style to a fixed value.

Screen.Recording.2025-02-14.at.13.32.20.mov

@planger
Copy link
Contributor

planger commented Feb 14, 2025

@atahankilc Thanks, yes there seems to be a rather severe bug on master in the chat input at the moment. I'll wait for it to be resolved and then review this PR rebased on the hopefully soon fixed master. Otherwise it is hard to verify the final behavior.

Also, I expect a few conflicts with #14787. So I may opt to merge the other one first and then I'll take over rebasing this one and resolving their conflicts, because this change is smaller, it is probably easier this way.

Code looks already good so far! Thank you!

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked #14942 and retested this. I think it works great now, thank you very much for your consistent work on this nice feature!

We could already merge this PR now and rebase #14787 --- I expect several conflicts though. Or we do it the other way around, which might be easier. I'll give it a try locally.

@planger planger merged commit b119b20 into eclipse-theia:master Feb 17, 2025
10 of 11 checks passed
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 17, 2025
@planger
Copy link
Contributor

planger commented Feb 17, 2025

Conflicts are straight forward to resolve, so I'll rebase #14787

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants